From: Matthieu Gallien Date: Thu, 12 Jun 2025 12:16:41 +0000 (+0200) Subject: fix(readonly): better handling of ACL on read-only files on windows X-Git-Tag: archive/raspbian/3.16.7-1_deb13u1+rpi1^2~13^2~1^2~3^2 X-Git-Url: https://dgit.raspbian.org/%22http:/www.example.com/cgi/%22https://%22Programmet/%22http:/www.example.com/cgi/%22https:/%22Programmet?a=commitdiff_plain;h=1823845baa2562ae33edfc872b599d35793edce9;p=nextcloud-desktop.git fix(readonly): better handling of ACL on read-only files on windows Signed-off-by: Matthieu Gallien --- diff --git a/src/common/filesystembase.cpp b/src/common/filesystembase.cpp index e2b4837f4..cb83dc576 100644 --- a/src/common/filesystembase.cpp +++ b/src/common/filesystembase.cpp @@ -35,6 +35,8 @@ #include #include #include +#include +#include #endif namespace OCC { @@ -148,6 +150,12 @@ void FileSystem::setFileReadOnly(const QString &filename, bool readonly) } } + if (!readonly) { + // current read-only folder ACL needs to be removed from files also when making a folder read-write + // we currently have a too limited set of authorization for files when applying the restrictive ACL for folders on the child files + setAclPermission(filename, FileSystem::FolderPermissions::ReadWrite, false); + } + return; #endif QFile file(filename); @@ -689,6 +697,157 @@ QString FileSystem::pathtoUNC(const QString &_str) } return QStringLiteral(R"(\\?\)") + str; } + +bool FileSystem::setAclPermission(const QString &unsafePath, FolderPermissions permissions, bool applyAlsoToFiles) +{ + SECURITY_INFORMATION info = DACL_SECURITY_INFORMATION; + std::unique_ptr securityDescriptor; + auto neededLength = 0ul; + + const auto path = longWinPath(unsafePath); + + const auto safePathFileInfo = QFileInfo{path}; + + if (!GetFileSecurityW(path.toStdWString().c_str(), info, nullptr, 0, &neededLength)) { + const auto lastError = GetLastError(); + if (lastError != ERROR_INSUFFICIENT_BUFFER) { + qCWarning(lcFileSystem) << "error when calling GetFileSecurityW" << path << lastError; + return false; + } + + securityDescriptor.reset(new char[neededLength]); + + if (!GetFileSecurityW(path.toStdWString().c_str(), info, securityDescriptor.get(), neededLength, &neededLength)) { + qCWarning(lcFileSystem) << "error when calling GetFileSecurityW" << path << GetLastError(); + return false; + } + } + + int daclPresent = false, daclDefault = false; + PACL resultDacl = nullptr; + if (!GetSecurityDescriptorDacl(securityDescriptor.get(), &daclPresent, &resultDacl, &daclDefault)) { + qCWarning(lcFileSystem) << "error when calling GetSecurityDescriptorDacl" << path << GetLastError(); + return false; + } + if (!daclPresent || !resultDacl) { + qCWarning(lcFileSystem) << "error when calling DACL needed to set a folder read-only or read-write is missing" << path; + return false; + } + + PSID sid = nullptr; + if (!ConvertStringSidToSidW(L"S-1-5-32-545", &sid)) + { + qCWarning(lcFileSystem) << "error when calling ConvertStringSidToSidA" << path << GetLastError(); + return false; + } + + ACL_SIZE_INFORMATION aclSize; + if (!GetAclInformation(resultDacl, &aclSize, sizeof(aclSize), AclSizeInformation)) { + qCWarning(lcFileSystem) << "error when calling GetAclInformation" << path << GetLastError(); + return false; + } + + const auto newAclSize = aclSize.AclBytesInUse + sizeof(ACCESS_DENIED_ACE) + GetLengthSid(sid); + qCDebug(lcFileSystem) << "allocated a new DACL object of size" << newAclSize; + + std::unique_ptr newDacl{reinterpret_cast(new char[newAclSize])}; + if (!InitializeAcl(newDacl.get(), newAclSize, ACL_REVISION)) { + const auto lastError = GetLastError(); + if (lastError != ERROR_INSUFFICIENT_BUFFER) { + qCWarning(lcFileSystem) << "insufficient memory error when calling InitializeAcl" << path; + return false; + } + + qCWarning(lcFileSystem) << "error when calling InitializeAcl" << path << lastError; + return false; + } + + if (permissions == FileSystem::FolderPermissions::ReadOnly) { + qCInfo(lcFileSystem) << path << "will be read only"; + + if (!AddAccessDeniedAceEx(newDacl.get(), ACL_REVISION, OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE, + FILE_DELETE_CHILD | DELETE | FILE_WRITE_DATA | FILE_WRITE_ATTRIBUTES | FILE_WRITE_EA | FILE_APPEND_DATA, sid)) { + qCWarning(lcFileSystem) << "error when calling AddAccessDeniedAce << path" << GetLastError(); + return false; + } + } + + if (permissions == FileSystem::FolderPermissions::ReadWrite) { + qCInfo(lcFileSystem) << path << "will be read write"; + } + + for (int i = 0; i < aclSize.AceCount; ++i) { + void *currentAce = nullptr; + if (!GetAce(resultDacl, i, ¤tAce)) { + qCWarning(lcFileSystem) << "error when calling GetAce" << path << GetLastError(); + return false; + } + + const auto currentAceHeader = reinterpret_cast(currentAce); + + if (permissions == FileSystem::FolderPermissions::ReadWrite && (ACCESS_DENIED_ACE_TYPE == (currentAceHeader->AceType & ACCESS_DENIED_ACE_TYPE))) { + qCWarning(lcFileSystem) << "AceHeader" << path << currentAceHeader->AceFlags << currentAceHeader->AceSize << currentAceHeader->AceType; + continue; + } + + if (!AddAce(newDacl.get(), ACL_REVISION, i + 1, currentAce, currentAceHeader->AceSize)) { + const auto lastError = GetLastError(); + if (lastError != ERROR_INSUFFICIENT_BUFFER) { + qCWarning(lcFileSystem) << "insufficient memory error when calling AddAce" << path; + return false; + } + + if (lastError != ERROR_INVALID_PARAMETER) { + qCWarning(lcFileSystem) << "invalid parameter error when calling AddAce" << path << "ACL size" << newAclSize; + return false; + } + + qCWarning(lcFileSystem) << "error when calling AddAce" << path << lastError << "acl index" << (i + 1); + return false; + } + } + + SECURITY_DESCRIPTOR newSecurityDescriptor; + if (!InitializeSecurityDescriptor(&newSecurityDescriptor, SECURITY_DESCRIPTOR_REVISION)) { + qCWarning(lcFileSystem) << "error when calling InitializeSecurityDescriptor" << path << GetLastError(); + return false; + } + + if (!SetSecurityDescriptorDacl(&newSecurityDescriptor, true, newDacl.get(), false)) { + qCWarning(lcFileSystem) << "error when calling SetSecurityDescriptorDacl" << path << GetLastError(); + return false; + } + + if (safePathFileInfo.isDir() && applyAlsoToFiles) { + const auto currentFolder = safePathFileInfo.dir(); + const auto childFiles = currentFolder.entryList(QDir::Filter::Files); + for (const auto &oneEntry : childFiles) { + const auto childFile = QDir::toNativeSeparators(path + QDir::separator() + oneEntry); + + const auto &childFileStdWString = childFile.toStdWString(); + const auto attributes = GetFileAttributes(childFileStdWString.c_str()); + + // testing if that could be a pure virtual placeholder file (i.e. CfApi file without data) + // we do not want to trigger implicit hydration ourself + if ((attributes & FILE_ATTRIBUTE_SPARSE_FILE) != 0) { + continue; + } + + if (!SetFileSecurityW(childFileStdWString.c_str(), info, &newSecurityDescriptor)) { + qCWarning(lcFileSystem) << "error when calling SetFileSecurityW" << childFile << GetLastError(); + return false; + } + } + } + + if (!SetFileSecurityW(QDir::toNativeSeparators(path).toStdWString().c_str(), info, &newSecurityDescriptor)) { + qCWarning(lcFileSystem) << "error when calling SetFileSecurityW" << QDir::toNativeSeparators(path) << GetLastError(); + return false; + } + + return true; +} + #endif } // namespace OCC diff --git a/src/common/filesystembase.h b/src/common/filesystembase.h index 481e765f6..ef90aedb4 100644 --- a/src/common/filesystembase.h +++ b/src/common/filesystembase.h @@ -176,6 +176,8 @@ namespace FileSystem { std::filesystem::perms OCSYNC_EXPORT filePermissionsWinSymlinkSafe(const QString &filename); std::filesystem::perms OCSYNC_EXPORT filePermissionsWin(const QString &filename); void OCSYNC_EXPORT setFilePermissionsWin(const QString &filename, const std::filesystem::perms &perms); + + bool OCSYNC_EXPORT setAclPermission(const QString &path, FileSystem::FolderPermissions permissions, bool applyAlsoToFiles); #endif /** diff --git a/src/libsync/filesystem.cpp b/src/libsync/filesystem.cpp index 3b06ff1d7..45a401ad3 100644 --- a/src/libsync/filesystem.cpp +++ b/src/libsync/filesystem.cpp @@ -363,144 +363,10 @@ bool FileSystem::setFolderPermissions(const QString &path, } #ifdef Q_OS_WIN - SECURITY_INFORMATION info = DACL_SECURITY_INFORMATION; - std::unique_ptr securityDescriptor; - auto neededLength = 0ul; - - if (!GetFileSecurityW(path.toStdWString().c_str(), info, nullptr, 0, &neededLength)) { - const auto lastError = GetLastError(); - if (lastError != ERROR_INSUFFICIENT_BUFFER) { - qCWarning(lcFileSystem) << "error when calling GetFileSecurityW" << path << lastError; - return false; - } - - securityDescriptor.reset(new char[neededLength]); - - if (!GetFileSecurityW(path.toStdWString().c_str(), info, securityDescriptor.get(), neededLength, &neededLength)) { - qCWarning(lcFileSystem) << "error when calling GetFileSecurityW" << path << GetLastError(); - return false; - } - } - - int daclPresent = false, daclDefault = false; - PACL resultDacl = nullptr; - if (!GetSecurityDescriptorDacl(securityDescriptor.get(), &daclPresent, &resultDacl, &daclDefault)) { - qCWarning(lcFileSystem) << "error when calling GetSecurityDescriptorDacl" << path << GetLastError(); - return false; - } - if (!daclPresent || !resultDacl) { - qCWarning(lcFileSystem) << "error when calling DACL needed to set a folder read-only or read-write is missing" << path; - return false; - } - - PSID sid = nullptr; - if (!ConvertStringSidToSidW(L"S-1-5-32-545", &sid)) - { - qCWarning(lcFileSystem) << "error when calling ConvertStringSidToSidA" << path << GetLastError(); - return false; - } - - ACL_SIZE_INFORMATION aclSize; - if (!GetAclInformation(resultDacl, &aclSize, sizeof(aclSize), AclSizeInformation)) { - qCWarning(lcFileSystem) << "error when calling GetAclInformation" << path << GetLastError(); - return false; - } - - const auto newAclSize = aclSize.AclBytesInUse + sizeof(ACCESS_DENIED_ACE) + GetLengthSid(sid); - qCDebug(lcFileSystem) << "allocated a new DACL object of size" << newAclSize; - - std::unique_ptr newDacl{reinterpret_cast(new char[newAclSize])}; - if (!InitializeAcl(newDacl.get(), newAclSize, ACL_REVISION)) { - const auto lastError = GetLastError(); - if (lastError != ERROR_INSUFFICIENT_BUFFER) { - qCWarning(lcFileSystem) << "insufficient memory error when calling InitializeAcl" << path; - return false; - } - - qCWarning(lcFileSystem) << "error when calling InitializeAcl" << path << lastError; - return false; - } - - if (permissions == FileSystem::FolderPermissions::ReadOnly) { - qCInfo(lcFileSystem) << path << "will be read only"; - - if (!AddAccessDeniedAceEx(newDacl.get(), ACL_REVISION, OBJECT_INHERIT_ACE | CONTAINER_INHERIT_ACE, - FILE_DELETE_CHILD | DELETE | FILE_WRITE_DATA | FILE_WRITE_ATTRIBUTES | FILE_WRITE_EA | FILE_APPEND_DATA, sid)) { - qCWarning(lcFileSystem) << "error when calling AddAccessDeniedAce << path" << GetLastError(); - return false; - } - } - - if (permissions == FileSystem::FolderPermissions::ReadWrite) { - qCInfo(lcFileSystem) << path << "will be read write"; - } - - for (int i = 0; i < aclSize.AceCount; ++i) { - void *currentAce = nullptr; - if (!GetAce(resultDacl, i, ¤tAce)) { - qCWarning(lcFileSystem) << "error when calling GetAce" << path << GetLastError(); - return false; - } - - const auto currentAceHeader = reinterpret_cast(currentAce); - - if (permissions == FileSystem::FolderPermissions::ReadWrite && (ACCESS_DENIED_ACE_TYPE == (currentAceHeader->AceType & ACCESS_DENIED_ACE_TYPE))) { - qCWarning(lcFileSystem) << "AceHeader" << path << currentAceHeader->AceFlags << currentAceHeader->AceSize << currentAceHeader->AceType; - continue; - } - - if (!AddAce(newDacl.get(), ACL_REVISION, i + 1, currentAce, currentAceHeader->AceSize)) { - const auto lastError = GetLastError(); - if (lastError != ERROR_INSUFFICIENT_BUFFER) { - qCWarning(lcFileSystem) << "insufficient memory error when calling AddAce" << path; - return false; - } - - if (lastError != ERROR_INVALID_PARAMETER) { - qCWarning(lcFileSystem) << "invalid parameter error when calling AddAce" << path << "ACL size" << newAclSize; - return false; - } - - qCWarning(lcFileSystem) << "error when calling AddAce" << path << lastError << "acl index" << (i + 1); - return false; - } - } - - SECURITY_DESCRIPTOR newSecurityDescriptor; - if (!InitializeSecurityDescriptor(&newSecurityDescriptor, SECURITY_DESCRIPTOR_REVISION)) { - qCWarning(lcFileSystem) << "error when calling InitializeSecurityDescriptor" << path << GetLastError(); - return false; - } - - if (!SetSecurityDescriptorDacl(&newSecurityDescriptor, true, newDacl.get(), false)) { - qCWarning(lcFileSystem) << "error when calling SetSecurityDescriptorDacl" << path << GetLastError(); - return false; - } - - auto currentFolder = QDir{path}; - const auto childFiles = currentFolder.entryList(QDir::Filter::Files); - for (const auto &oneEntry : childFiles) { - const auto childFile = QDir::toNativeSeparators(path + QDir::separator() + oneEntry); - - const auto &childFileStdWString = childFile.toStdWString(); - const auto attributes = GetFileAttributes(childFileStdWString.c_str()); - - // testing if that could be a pure virtual placeholder file (i.e. CfApi file without data) - // we do not want to trigger implicit hydration ourself - if ((attributes & FILE_ATTRIBUTE_SPARSE_FILE) != 0) { - continue; - } - - if (!SetFileSecurityW(childFileStdWString.c_str(), info, &newSecurityDescriptor)) { - qCWarning(lcFileSystem) << "error when calling SetFileSecurityW" << childFile << GetLastError(); - return false; - } - } - - if (!SetFileSecurityW(QDir::toNativeSeparators(path).toStdWString().c_str(), info, &newSecurityDescriptor)) { - qCWarning(lcFileSystem) << "error when calling SetFileSecurityW" << QDir::toNativeSeparators(path) << GetLastError(); - return false; - } + // current read-only folder ACL needs to be removed from files also when making a folder read-write + // we currently have a too limited set of authorization for files when applying the restrictive ACL for folders on the child files + setFileReadOnly(path, permissions == FileSystem::FolderPermissions::ReadOnly); + setAclPermission(path, permissions, permissions == FileSystem::FolderPermissions::ReadWrite ? true : false); permissionsDidChange = true; #else diff --git a/test/testpermissions.cpp b/test/testpermissions.cpp index fb16bb1e4..ee4507c10 100644 --- a/test/testpermissions.cpp +++ b/test/testpermissions.cpp @@ -434,11 +434,7 @@ private slots: currentLocalState = fakeFolder.currentLocalState(); count++; } -#if defined Q_OS_WINDOWS - QCOMPARE(count, 0); -#else QCOMPARE(count, 2); -#endif QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState()); }